claude-code-review: run on Claude-bot pushes#900
Conversation
The job `if:` and mirrored `concurrency.cancel-in-progress` expression skipped every bot-actor push (generic `endsWith(actor, '[bot]')` / `sender.type == 'Bot'` rule). That silently excluded Claude's own commits: Claude Code on the web and the @claude agent commit as the `claude` user account, and Claude GitHub App pushes surface as `claude[bot]` — exactly the AI-authored diffs we most want reviewed. Add explicit clauses so actor `claude` / `claude[bot]` pushes trigger a review. Recursion-safe: the reviewer is review-only (the `--disallowedTools Bash(git commit:*) …` list means it can never commit or push), so it can never fire a self-triggering `synchronize`; and the @claude agent pushes via GITHUB_TOKEN, which never fires `synchronize` (claude.yml dispatches the review explicitly). Both expressions kept in sync per the existing invariant.
|
@claude review |
|
👀 Picked up by workflow run #27191708084. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
@claude review |
|
👀 Picked up by workflow run #27598978484. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
d-morrison
left a comment
d-morrison
left a comment
There was a problem hiding this comment.
Review
Posting this directly: the automated @claude review dispatch hasn't delivered a review on this PR (no review/comment from either of the two prior @claude review requests — see note at the bottom), so here it is from the subscribed session.
Verdict: safe to merge. The change is purely additive to two boolean expressions — it only ever causes more review runs, never fewer, and the workflow stays review-only — so the downside risk is low. Two substantive points and one coupling note, in priority order.
1. Verify the actor string is actually claude[bot] (the load-bearing clause could be a no-op)
The whole change hinges on a real Claude-bot synchronize/pull_request push presenting as github.actor == 'claude[bot]'. That assumption isn't verified anywhere — and the PR description's evidence (commits authored by user claude, id 81847) doesn't actually establish it: github.actor is the pusher that triggered the run, not the commit author. They differ for App-mediated pushes.
Concretely: PR #706 was opened by the anthropic-code-agent App. If that App's pushes surface as anthropic-code-agent[bot] (plausible, given the App slug), then neither new clause matches and the change silently does nothing for exactly the PRs it targets.
Recommend confirming against a real run before relying on this — e.g. check github.actor on an actual claude-pushed synchronize event (Actions run → triggering actor), or temporarily log ${{ github.actor }}. If it turns out to be a different bot slug, switch to that string (or a startsWith(github.actor, 'claude') || contains(github.actor, 'anthropic')-style match).
2. The github.actor == 'claude' clause is almost certainly redundant
claude (id 81847) is a regular User account, so its pushes have sender.type == 'User' and an actor not ending in [bot] — which means the existing middle clause (github.event.sender.type != 'Bot' && !endsWith(github.actor, '[bot]')) already admits them. So of the two added clauses, only claude[bot] changes behavior; == 'claude' is dead unless that account is ever typed as a Bot.
Not harmful, but either drop it or relabel it in the comment as a deliberate belt-and-suspenders so a future reader doesn't think it's load-bearing.
3. Loop-prevention now depends on the Bash(git commit:*) disallow — make the coupling explicit
The recursion-safety argument is sound today, but it now rests entirely on the review step's --disallowedTools "...Bash(git commit:*)..." list. Before this PR, the bot-actor filter was a second, independent guard against the reviewer re-triggering itself; this change removes that backstop for claude[bot]. If someone later loosens disallowedTools, the self-trigger loop the bot-filter used to prevent comes back silently. Worth a one-line cross-reference in the disallowedTools block ("also load-bearing for review-loop prevention — see the job if: Claude-bot clause") so the two stay tied together.
Nits
- The CI
claude-reviewfailure on this PR is the expected OIDC workflow-self-validation error (the action refuses to mint a token when the PR edits its own workflow file); it clears on merge tomain. Not a defect — already documented in the PR body.
Nothing here blocks merge; #1 is the one I'd resolve first, since if the actor string is wrong the PR doesn't accomplish its goal even though everything looks correct.
— review posted directly by the session subscribed to this PR, since the automated reviewer didn't fire.
Generated by Claude Code
|
👀 Picked up by workflow run #27599125716. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
@claude fix nits |
|
👀 Picked up by workflow run #27634862864. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
API Error: 500 Internal server error. This is a server-side issue, usually temporary — try again in a moment. If it persists, check https://status.claude.com. — posted by @claude post-step from workflow run |
Address review nits on PR #900 (the @claude fix-nits run aborted on a transient API 500 before applying them). - Clarify in the job `if:` comment that `github.actor == 'claude[bot]'` is the load-bearing clause, while `github.actor == 'claude'` is deliberate belt-and-suspenders: the `claude` user (id 81847) is a regular User account already admitted by the existing middle clause, so the explicit clause only matters if that account is ever reclassified as a Bot. Prevents a future reader from treating it as the thing that enables review of Claude pushes. - Cross-reference the recursion-safety coupling in the `disallowedTools` block: `Bash(git commit:*)` is now also load-bearing for the job `if:` Claude-bot clause's review-loop prevention, so loosening the disallow-list requires revisiting that clause. Comment-only: the job `if:` and concurrency.cancel-in-progress expressions are unchanged and remain byte-identical (verified).
|
The Both are comment-only — the job
Not addressed (it's a verification, not a comment nit): the review's point #1 — confirming that a real Claude App push actually presents as The expected Generated by Claude Code |
|
👀 Picked up by workflow run #27646413589. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
Actor string verified — the While investigating, I found direct evidence from an existing workflow run in this repo:
So review concern #1 is resolved: the actor string for Claude App-mediated pushes in this repo is (This run was triggered by a false-positive: the previous status comment contained the literal text |
|
The The investigation is complete. Here's a summary of the findings posted to PR #900: Actor string verified: Workflow run 27032683838 was triggered by commit Additionally, the API response from posting the comment revealed that the Claude GitHub App slug is literally The PR is ready to merge. No code changes were needed for this run. — posted by @claude post-step from workflow run |
|
👀 Picked up by workflow run #27646992663. R/Quarto/renv setup runs first (~3-5 min); Claude itself responds after that. |
|
Closing this in favor of #911. Investigation here showed this PR's actor-filter approach is effectively a no-op: Claude's PR pushes arrive as actor The actual reason Claude-authored commits weren't getting reviewed is a broken dispatch: The nit cleanups discussed here (commit 6e88fc4) are no longer needed since the change itself is being dropped. Generated by Claude Code |
2 participants
What
Make the Claude Code Review workflow (
.github/workflows/claude-code-review.yml) run on Claude-bot commits, which it was silently skipping.Why
The job
if:(and the mirroredconcurrency.cancel-in-progressexpression) skipped every bot-actor push via the genericendsWith(github.actor, '[bot]')/github.event.sender.type == 'Bot'rule. That rule was added to stop other bots' pushes from triggering reviews, but it also excluded Claude's own commits — which are exactly the AI-authored diffs we most want a second pass over.Inspecting recent Claude-authored commits in this repo, they are committed under:
claudeuser account (loginclaude, id81847,noreply@anthropic.com) — used by Claude Code on the web and the@claudeagent, andclaude[bot]when a push goes through the Claude GitHub App.Both forms were being filtered out, so a Claude push to a PR left it stuck on whatever verdict existed before.
Change
Add two clauses to both the job
if:and theconcurrency.cancel-in-progressexpression (kept identical, per the existing in-sync invariant):Why this is recursion-safe
The original bot filter existed partly to avoid the self-trigger / frozen-comment loop documented in the concurrency comment (PR #809). Letting Claude-bot pushes through does not reintroduce it:
--disallowedTools "Bash(git commit:*)" …list means the reviewer can never commit, hence never push, hence can never fire asynchronizeevent that re-triggers itself.@claudeagent's own commits are pushed withGITHUB_TOKEN, which by design does not firesynchronize(claude.ymldispatches this workflow explicitly viaworkflow_dispatchinstead), so those are not double-reviewed.The load-bearing comments in both spots were updated to explain the exception and the safety argument.
Verification
python3 -c "import yaml; ..."parses the workflow and confirms theif:andcancel-in-progressexpressions are byte-identical (the comments require them to stay in sync).Per CLAUDE.md, this is a
.github/CI/infra-only change kept in its own dedicated PR (no book content touched).Known CI note —
claude-reviewcheck fails on this PR (expected)The
claude-reviewcheck fails here with:This is inherent to editing
claude-code-review.ymlitself, not a defect in this change.anthropics/claude-code-action's OIDC app-token exchange requires the workflow file on the PR branch to be byte-identical to the one on the default branch (a guard against a PR modifying the workflow to exfiltrate the app token). Any PR that touches this workflow trips it. It clears automatically once this merges tomain(then branch == default). The fix cannot be applied on the PR without reverting the change.https://claude.ai/code/session_01ER1dFrUPTjaHeL3n7Yy7qG